Skip to content

chore: Triage and clean up TODO comments - #473

Merged
currantw merged 33 commits into
valkey-io:mainfrom
currantw:325-todos
Jul 29, 2026
Merged

chore: Triage and clean up TODO comments#473
currantw merged 33 commits into
valkey-io:mainfrom
currantw:325-todos

Conversation

@currantw

@currantw currantw commented Jul 23, 2026 •
edited
Loading

Copy link
Copy Markdown
Collaborator

Summary

Triage and clean up TODO comments across the codebase. Adds a check-todos script and CI workflow to enforce that all TODOs reference an open GitHub issue with a description, then resolves or annotates all existing TODOs.

Issue Link

Closes #325.

Features and Behaviour Changes

TODO checker infrastructure

  • Add dev/scripts/check_todos.py — validates that every TODO comment follows the format TODO #<number>: <description>.
    • Description must be at least 10 characters.
    • Referenced issue must be open.
  • Add dev/conf/check-todos-ignore — fnmatch-based ignore file for paths excluded from validation.
  • Add .github/workflows/check-todos.yml — CI workflow that runs the checker on PRs and pushes.
  • Add task check:todos command.
  • Document the TODO format in DEVELOPER.md.

TODO comment fixes and removals

  • Request.StreamCommands.cs — Remove SER-specific StreamTrimAsync method; replace with GLIDE-style method accepting StreamTrimOptions. SER Database.StreamCommands now routes through the BaseClient.StreamTrimAsync GLIDE interface method.
  • IDatabaseAsync.StreamCommands.cs — Fix StreamTrimAsync signature to use non-nullable long maxLength (matching StackExchange.Redis 2.8.58).
  • IBaseClient.StreamCommands.cs / BaseClient.StreamCommands.cs — Uncomment and implement StreamTrimAsync(ValkeyKey, StreamTrimOptions).
  • ScanOptions.cs — Add seealso links for SCAN, SSCAN, and ZSCAN commands.
  • Request.cs — Extract shared ToValkeyKeyArray and ToValkeyValueArray collection converters.
  • Request.SetCommands.cs — Extract ConvertSetScanResponse for SSCAN response parsing.
  • Request.SortedSetCommands.cs — Extract ConvertSortedSetScanResponse and ToSortedSetEntriesFromFlatArray for ZSCAN response parsing; remove old ToValkeyValues delegate.
  • Request.GenericCommands.cs — Extract ConvertClusterScanResponse for top-level SCAN (string cursor for cluster compatibility).
  • GlideClusterClient.cs — Refactor ClusterScanCommand to accept ClusterScanCursor instead of raw string; simplify ScanAsync loop.
  • GlideString.cs — Replace round-trip UTF-8 encode/decode with Utf8.IsValid (net8.0 API, allocation-free).
  • Helpers.cs — Remove dead code (DownCastVals, ConvertKeyValuePairsToArray).
  • GlideClient.cs / GlideClusterClient.cs — Remove // TODO add pubsub comments; docs already updated in prior commits.
  • ConnectionMultiplexer.cs — Remove TODO referencing closed upstream issue ([Feature request] Routing on standalone valkey-glide#4293).
  • FFI.structs.cs — Replace vague TODO with issue reference (TODO #472).
  • Valkey.Glide.nuspec — Resolve all packaging TODOs: add <releaseNotes>, commit attribute, remove stale comments.
  • cd.yml — Pass commit=${{ github.sha }} to nuspec via NuspecProperties.
  • Integration/unit tests — Remove resolved TODO skip markers; update CommandTests for new StreamTrimAsync signature.

Cleanup

  • Move dev/coverage/coverage-baseline.json to dev/conf/coverage-baseline.json.
  • Move dev/coverage/.runsettings to dev/conf/coverage-runsettings.xml.
  • Remove empty dev/coverage/ directory (output subdirectories are recreated at runtime).
  • Update all references in CI workflows, scripts, and documentation.

Implementation

The main design decisions:

  1. Stream trim refactoring: SER methods now build StreamTrimOptions (MaxLen or MinId) and call through the GLIDE BaseClient.StreamTrimAsync method, eliminating direct Request calls from the SER layer.
  2. Scan response converters: Each scan command (SCAN, SSCAN, ZSCAN) has its own dedicated converter method rather than a shared generic — simpler to read and avoids unnecessary indirection.
  3. TODO checker: Uses git grep for discovery, gh issue view for validation, and fnmatch patterns for exclusions. Fails hard if the ignore file is missing.

Limitations

⚪ None

Testing

  • All 547 unit tests pass.
  • Integration tests not run locally (require a running Valkey instance).
  • TODO checker validates all 25 remaining TODOs pass format and issue checks.

Related Issues

Checklist

  • This Pull Request is related to one issue.
  • Commit message has a detailed description of what changed and why.
  • Tests are added or updated and all checks pass.
  • CHANGELOG.md, README.md, DEVELOPER.md, and other documentation files are updated.
  • Destination branch is correct - main or release
  • Create merge commit if merging release branch into main, squash otherwise.
  • All TODO comments referencing issue(s) closed by this PR have been resolved.

@currantw currantw self-assigned this Jul 23, 2026
@currantw currantw changed the title chore: Triage and clean up TODO comments (#325) chore: Triage and clean up TODO comments Jul 23, 2026
@currantw currantw added docs Documentation and developer guides ci CI/CD pipelines and GitHub Actions dev Internal changes like cleanup, refactoring, or developer tools. labels Jul 23, 2026
Copilot AI reviewed Jul 23, 2026
View reviewed changes

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR introduces repository-wide enforcement for TODO comment formatting (including validating referenced GitHub issues), while also cleaning up and refactoring several command implementations (notably stream trim and scan variants) and updating related docs/CI configuration.

Changes:

  • Added a TODO checker (dev/scripts/check_todos.py), ignore configuration, Taskfile integration, and a dedicated CI workflow to enforce TODO #<issue>: <description> formatting.
  • Refactored stream trim and scan command plumbing (request builders, converters, and client/DB/IServer iteration patterns) and updated unit/integration tests accordingly.
  • Cleaned up TODOs and updated documentation/CI paths (coverage config relocation, nuspec metadata, workflow/task renames).

Reviewed changes

Copilot reviewed 45 out of 47 changed files in this pull request and generated 4 comments.

Show a summary per file
File Description
tests/Valkey.Glide.UnitTests/CommandTests.cs Updates stream trim request arg/converter tests to use StreamTrimOptions.
tests/Valkey.Glide.IntegrationTests/StreamCommandTests.cs Removes commented legacy TODO markers in stream integration tests.
tests/Valkey.Glide.IntegrationTests/StandaloneClientTests.cs Removes TODO commentary from an integration test.
tests/Valkey.Glide.IntegrationTests/SortedSetCommandTests.cs Replaces version-gated skips with centralized Skip helpers.
tests/Valkey.Glide.IntegrationTests/Skip.cs Renames/organizes version-based skip helpers and messages.
tests/Valkey.Glide.IntegrationTests/SharedCommandTests.cs Removes a TODO and keeps current assertion behavior.
tests/Valkey.Glide.IntegrationTests/SharedBatchTests.cs Replaces custom-command placeholders with concrete batch command calls.
tests/Valkey.Glide.IntegrationTests/HashCommandTests.cs Removes a TODO comment from the test class header.
Taskfile.yml Adds task check umbrella and renames check tasks; adds --yes to npx invocations.
sources/Valkey.Glide/Valkey.Glide.nuspec Adds repo commit metadata + release notes; clarifies native binary packaging expectations.
sources/Valkey.Glide/Internals/Request.StreamCommands.cs Replaces legacy stream trim request builder with options-based trim args.
sources/Valkey.Glide/Internals/Request.SortedSetCommands.cs Refactors sorted set scan response conversion and reuses shared value-array converter.
sources/Valkey.Glide/Internals/Request.SetCommands.cs Refactors set scan response conversion and reuses shared value-array converter.
sources/Valkey.Glide/Internals/Request.GenericCommands.cs Extracts cluster scan response converter and reuses shared key-array converter.
sources/Valkey.Glide/Internals/Request.cs Removes internal SCAN arg builder; adds shared key/value array converters.
sources/Valkey.Glide/Internals/Helpers.cs Removes dead helpers and adds SkipAsync for async sequences.
sources/Valkey.Glide/Internals/FFI.structs.cs Replaces vague TODOs with an issue-linked TODO and removes a stale config TODO.
sources/Valkey.Glide/GlideString.cs Uses allocation-free UTF-8 validity checking before decoding.
sources/Valkey.Glide/Errors.cs Removes a stale TODO from ConfigurationError.
sources/Valkey.Glide/Commands/Options/ScanOptions.cs Adds docs links and moves scan argument construction onto ScanOptions.
sources/Valkey.Glide/Client/IBaseClient.StreamCommands.cs Adds and documents the StreamTrimAsync(key, StreamTrimOptions) API.
sources/Valkey.Glide/Client/GlideClusterClient.cs Simplifies scan loop and introduces ClusterScanCursor usage.
sources/Valkey.Glide/Client/GlideClient.cs Simplifies scan loop and removes outdated doc TODO content.
sources/Valkey.Glide/Client/BaseClient.StreamCommands.cs Implements StreamTrimAsync(key, StreamTrimOptions) through Request.
sources/Valkey.Glide/Client/BaseClient.SortedSetCommands.cs Refactors sorted set scan iteration variable naming and loop structure.
sources/Valkey.Glide/Client/BaseClient.SetCommands.cs Refactors set scan iteration variable naming and loop structure.
sources/Valkey.Glide/Abstract/ValkeyServer.cs Refactors KeysAsync to use ScanOptions + async sequence skipping.
sources/Valkey.Glide/Abstract/IDatabaseAsync.StreamCommands.cs Adjusts stream trim overload signature (non-nullable long maxLength) and formatting.
sources/Valkey.Glide/Abstract/Database.StreamCommands.cs Routes stream trim through StreamTrimOptions and updates overload wiring.
sources/Valkey.Glide/Abstract/Database.SortedSetCommands.cs Refactors SortedSetScanAsync to use ScanOptions + async sequence skipping.
sources/Valkey.Glide/Abstract/Database.SetCommands.cs Refactors SetScanAsync to use ScanOptions + async sequence skipping.
sources/Valkey.Glide/Abstract/ConnectionMultiplexer.cs Cleans up TODO commentary and ensures subscription disposal is enclosed in braces.
docs/coverage.md Updates coverage baseline paths and documents new conf layout.
DEVELOPER.md Updates developer commands, adds TODO format documentation, and reorganizes checks/lint/format sections.
dev/scripts/get_licenses_from_ort.py Removes a TODO about license expression parsing.
dev/scripts/check_todos.py Adds new TODO-format and issue-state validation script.
dev/scripts/check_examples.py Updates docstring to match new script path and clarifies behavior.
dev/scripts/_constants.py Adds repo constant and moves coverage conf paths under dev/conf.
dev/conf/coverage-runsettings.xml Adds relocated coverage runsettings file.
dev/conf/coverage-baseline.json Adds relocated coverage baseline file.
dev/conf/check-todos-ignore Adds ignore patterns for the TODO checker.
AGENTS.md Updates quality gate checklist to reference the new task check and docs anchors.
.github/workflows/test.yml Updates coverage baseline path references.
.github/workflows/check-todos.yml Adds CI workflow to run TODO validation.
.github/workflows/check-examples.yml Updates task invocation to task check:examples.
.github/workflows/cd.yml Passes git SHA into nuspec properties as commit.
.github/pull_request_template.md Adds a checkbox ensuring TODOs referencing closed issues are resolved.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread sources/Valkey.Glide/Abstract/ValkeyServer.cs Outdated
Comment thread sources/Valkey.Glide/Abstract/Database.SetCommands.cs Outdated
Comment thread sources/Valkey.Glide/Abstract/Database.SortedSetCommands.cs Outdated
Comment thread sources/Valkey.Glide/Abstract/Database.StreamCommands.cs Outdated
@currantw

currantw commented Jul 23, 2026

Copy link
Copy Markdown
Collaborator Author

Sample output from recent run:

task: [check:todos] python3 dev/scripts/check_todos.py
Checking TODO comments...

  FAIL  sources/Valkey.Glide/Client/BaseClient.StreamCommands.cs:98
        missing description (expected format: TODO #<number>: <description>)

Checked 27 TODOs: 26 passed, 1 failed.
task: Failed to run task "check:todos": exit status 1
Error: exit status 1

Error: Process completed with exit code 201.

currantw added 14 commits July 23, 2026 14:03
@currantw
feat: Add check-todos task and GitHub workflow
be8ba90 
Add a Python script that validates all TODO comments reference an open
GitHub issue. TODOs must use the format `TODO #<number>` inside a comment
marker (// # /* * <!--). The script checks each referenced issue via the
`gh` CLI and reports distinct errors for missing references, invalid IDs,
and closed issues.

- Add `dev/scripts/check_todos.py` with `_find_todos` and `_validate_todos`
- Add `task check-todos` to Taskfile.yml
- Add `.github/workflows/check-todos.yml` CI workflow
- Add TODO resolution checklist item to PR template
- Add `GITHUB_REPO` constant to `dev/scripts/_constants.py`
- Update AGENTS.md quality gates with task commands
- Consolidate DEVELOPER.md lint/format/checks section

Addresses GitHub issue valkey-io#325

Signed-off-by: Taylor Curran <taylor.curran@valkey.io>
Signed-off-by: currantw <taylor.curran@improving.com>
@currantw
refactor: Rename check tasks to use colon convention and split docs
20ef797 
- Rename check-examples, check-links, check-todos to check:examples,
  check:links, check:todos (consistent with lint:* and format:*)
- Add `task check` aggregator to run all checks in parallel
- Split DEVELOPER.md section into separate Formatting, Linting, and
  Checks headings
- Update AGENTS.md quality gates with separate section references
- Update workflow files to use new task names

Signed-off-by: Taylor Curran <taylor.curran@valkey.io>
Signed-off-by: currantw <taylor.curran@improving.com>
@currantw
fix: Add --yes to npx calls to prevent interactive prompts
7fa5147 
npx prompts for confirmation when a package isn't cached, which stalls
CI and automated runs. Adding --yes auto-accepts the install.

Signed-off-by: Taylor Curran <taylor.curran@valkey.io>
Signed-off-by: currantw <taylor.curran@improving.com>
@currantw
First batch of TODO comment fixes and removals.
5eb761b 
Signed-off-by: currantw <taylor.curran@improving.com>
@currantw
Second batch of TODO comment fixes and removals.
35ac7de 
Signed-off-by: currantw <taylor.curran@improving.com>
@currantw
Third batch of TODO comment fixes and removals.
2e68930 
Signed-off-by: currantw <taylor.curran@improving.com>
@currantw
feat(check-todos): add ignore file support for TODO checker
ac1376b 
Add dev/conf/check-todos-ignore to specify files excluded from TODO
validation using fnmatch glob patterns. This prevents the checker from
flagging its own comments.

Signed-off-by: currantw <taylor.curran@improving.com>
@currantw
Fourth batch of TODO comment fixes and removals.
67fc692 
Signed-off-by: currantw <taylor.curran@improving.com>
@currantw
Add a minimum description length
287825e 
Signed-off-by: currantw <taylor.curran@improving.com>
@currantw
refactor: Move coverage config files to dev/conf
17a5de9 
Move coverage-baseline.json and .runsettings from dev/coverage/ to
dev/conf/ (renamed to coverage-runsettings.xml) to consolidate config
files. Update all references in CI workflows, scripts, and docs.

Also add TODO format documentation and description validation to the
check-todos script.

Signed-off-by: currantw <taylor.curran@improving.com>
@currantw
refactor: Simplify scan methods to delegate to GLIDE internals
6842f36 
- Add protected cursor-accepting overloads to BaseClient for
  SetScanAsync and SortedSetScanAsync
- Add internal ScanKeysAsync to GlideClient for key scanning
- SER scan methods now delegate to GLIDE methods with SkipAsync
  for pageOffset support
- ValkeyServer.KeysAsync uses private ScanWithRouteAsync to
  preserve per-node routing via MakeRoute()

Signed-off-by: Taylor Curran <taylor.curran@valkey.io>
Signed-off-by: currantw <taylor.curran@improving.com>
@currantw
fix: Fix StreamTrimAsync infinite recursion and add guard clauses
3714bc2 
The int overload of StreamTrimAsync called itself due to parameter
resolution. Cast to long to target the correct overload, and add
ThrowIfNegative validation for maxLength.

- Add GuardClauses.ThrowIfNegative(long, string)
- Add GuardClauses.ThrowIfUnsupported(StreamTrimMode)
- Move ThrowIfUnsupportedTrimMode logic to GuardClauses

Signed-off-by: Taylor Curran <taylor.curran@valkey.io>
Signed-off-by: currantw <taylor.curran@improving.com>
@currantw
Fix TODO and add regions for stream commands
b834491 
Signed-off-by: currantw <taylor.curran@improving.com>
@currantw
refactor: Relax TODO pattern to match without comment prefix
1405c3b 
Remove the comment-prefix requirement from _TODO_PATTERN so TODOs
inside block comments (e.g. /* TODO */) are also caught. Handle
false positives from documentation files via check-todos-ignore.

Also removes the stale TODO from rust/src/ffi.rs (fields are now
implemented).

Signed-off-by: Taylor Curran <taylor.curran@valkey.io>
Signed-off-by: currantw <taylor.curran@improving.com>
currantw
currantw commented Jul 23, 2026
View reviewed changes
Comment thread .github/workflows/cd.yml
currantw
currantw commented Jul 23, 2026
View reviewed changes
Comment thread dev/conf/coverage-baseline.json
currantw
currantw commented Jul 23, 2026
View reviewed changes
Comment thread dev/scripts/get_licenses_from_ort.py
currantw
currantw commented Jul 23, 2026
View reviewed changes
Comment thread sources/Valkey.Glide/Abstract/ConnectionMultiplexer.cs
xShinnRyuu
xShinnRyuu reviewed Jul 24, 2026
View reviewed changes
Comment thread dev/scripts/check_todos.py Outdated
Comment thread sources/Valkey.Glide/Errors.cs
Comment thread rust/src/ffi.rs
Comment thread sources/Valkey.Glide/Abstract/IDatabaseAsync.StreamCommands.cs
Comment thread sources/Valkey.Glide/Abstract/Database.SetCommands.cs
Comment thread dev/scripts/check_todos.py
Comment thread dev/scripts/check_todos.py Outdated
Comment thread dev/scripts/check_todos.py Outdated
Comment thread dev/scripts/check_todos.py
Comment thread sources/Valkey.Glide/Client/BaseClient.SetCommands.cs
currantw added 6 commits July 27, 2026 09:54
@currantw
Make SetScanAsync and SortedSetScanAsync private protected.
061a7f8 
Signed-off-by: currantw <taylor.curran@improving.com>
@currantw
Merge branch 'main' of https://github.com/valkey-io/valkey-glide-csharp…
35148dc 
… into 325-todos
@currantw
Split TODO regexes
7ff432a 
Signed-off-by: currantw <taylor.curran@improving.com>
@currantw
Add space before TODO, simplify check_todos to enforce format
ec9355d 
Signed-off-by: currantw <taylor.curran@improving.com>
@currantw
Add GuardClauses.ThrowIfNotPositive and update scan methods to throw …
43e18ec 
…if negative page offset

Signed-off-by: currantw <taylor.curran@improving.com>
@currantw
Make _TODO_VALIDATION_PATTERN case sensitive
2361673 
Signed-off-by: currantw <taylor.curran@improving.com>
This was referenced Jul 27, 2026
Open
Open
currantw added 2 commits July 27, 2026 12:27
@currantw
Add TODOs for unused configurations.
b2e6bf1 
Signed-off-by: currantw <taylor.curran@improving.com>
@currantw
Clarify no inline comments
7ce7aa3 
Signed-off-by: currantw <taylor.curran@improving.com>
@currantw currantw mentioned this pull request Jul 27, 2026
Open
currantw added 2 commits July 27, 2026 13:10
@currantw
fix: Restore StreamTrimAsync(long?) signature to avoid breaking change
4374ee5 
Keep the previous `long? maxLength = null` signature on the extended
StreamTrimAsync overload to maintain backward compatibility. Add
ArgumentNullException for null maxLength (previously would have hit a
server syntax error). Add TODO valkey-io#486 to align with SER signature in 2.0.

Signed-off-by: Taylor Curran <taylor.curran@valkey.io>
Signed-off-by: currantw <taylor.curran@improving.com>
@currantw
Update docs
ec76f23 
Signed-off-by: currantw <taylor.curran@improving.com>
@currantw
currantw requested a review from xShinnRyuu July 27, 2026 20:19
xShinnRyuu
xShinnRyuu approved these changes Jul 27, 2026
View reviewed changes
Comment thread dev/scripts/check_todos.py Outdated
@currantw
Add TODO to "Check issue state"
5545a96 
Signed-off-by: currantw <taylor.curran@improving.com>
jamesx-improving
jamesx-improving reviewed Jul 27, 2026
View reviewed changes

@jamesx-improving jamesx-improving left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice cleanup — check_todos + CI wiring is a good forcing function, and the SER→GLIDE stream-trim refactor reads well. One inline note on the SkipAsync change.

Happy to re-review once you've had a look.

Comment thread sources/Valkey.Glide/Internals/Helpers.cs
currantw added 4 commits July 27, 2026 21:09
@currantw
Update ConnectionManagementCommandTests to use ServerFixture
437606a 
Signed-off-by: currantw <taylor.curran@improving.com>
@currantw
Use DisableParallelization = true
c26d8f8 
Signed-off-by: currantw <taylor.curran@improving.com>
@currantw
Fixed shared batch test failure
8e69d47 
Signed-off-by: currantw <taylor.curran@improving.com>
@currantw
Slight branch coverage decrease because of simplification (fewer bran…
bcaebc6 
…ches!)

Signed-off-by: currantw <taylor.curran@improving.com>
jamesx-improving
jamesx-improving approved these changes Jul 29, 2026
View reviewed changes

@jamesx-improving jamesx-improving left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Docs match the implementation now and the pageOffset > pageSize tests pin the choice — LGTM.

@currantw
docs: Document intentional pageOffset divergence from StackExchange.R…
43a7aab 
…edis

GLIDE skips pageOffset elements from the entire result stream
(deterministic), unlike SE.Redis which only skips within the first
SCAN batch (non-deterministic since batch sizes are server-controlled).

- Update XML docs on SetScanAsync, SortedSetScanAsync, and KeysAsync
- Add integration tests for pageOffset > pageSize edge case
- Remove duplicate KeysAsync_CommandFlags_Throws (already in CommandFlagsTests)

Signed-off-by: currantw <taylor.curran@improving.com>
@currantw
currantw merged commit b2c4a6f into valkey-io:main Jul 29, 2026
14 checks passed
@currantw
currantw deleted the 325-todos branch July 29, 2026 23:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

Copilot code review Copilot Copilot left review comments
@xShinnRyuu xShinnRyuu xShinnRyuu approved these changes
@jamesx-improving jamesx-improving jamesx-improving approved these changes

Assignees

@currantw currantw

Labels

ci CI/CD pipelines and GitHub Actions dev Internal changes like cleanup, refactoring, or developer tools. docs Documentation and developer guides

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

chore: Triage and clean up TODO comments across the codebase

4 participants

@currantw @xShinnRyuu @jamesx-improving